Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: calculate constraints all at once #300

Merged
merged 2 commits into from
Aug 23, 2022
Merged

refactor: calculate constraints all at once #300

merged 2 commits into from
Aug 23, 2022

Conversation

dmuldrew
Copy link
Collaborator

@dmuldrew dmuldrew commented Jul 26, 2022 •
edited
Loading

Pull Request doc

Purpose

This PR refactors the constraints function implementation to reduce the percentage of overall runtime from 48% to about 10%. This change appears to result in a 30-40% improvement in overall runtime. This PR also moves the call to linprog into the main loop which reduces the optimization code from about 20% of the overall runtime to 8%. These two changes together result in a roughly 50% overall improvement in the runtime of the algorithm.

I also created two small testing datasets in the PR which consist of data for 5 cars:
https://github.com/Breakthrough-Energy/PreREISE/blob/dmuldrew/refactor_calculate_constraints/prereise/gather/demanddata/transportation_electrification/tests/test_census_data.csv
and 30 cars:
https://github.com/Breakthrough-Energy/PreREISE/blob/dmuldrew/refactor_calculate_constraints/prereise/gather/demanddata/transportation_electrification/tests/profiling_census_data.csv

What the code is doing

Calculates constraints over the entire dataframe instead of calculating constraints for individual trip data within a double loop. Cost is still calculated within the loops since it is dependent on previous iterations.

Testing

manual performance testing and automated integration testing

Where to look

The performance-related changes are in smart_charging.py.

Time estimate

~15min

@dmuldrew dmuldrew self-assigned this Jul 26, 2022
rouille
rouille reviewed Jul 26, 2022
View reviewed changes
Pipfile Outdated Show resolved Hide resolved
rouille
rouille reviewed Jul 26, 2022
View reviewed changes
prereise/gather/demanddata/transportation_electrification/const.py Outdated
@@ -36,4 +36,5 @@

safety_coefficient = 1

# ???
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note to myself that we need to ask what the ER variable is?

@rouille
Copy link
Collaborator

rouille commented Jul 26, 2022 •
edited
Loading

It is out of the scope of this PR but why segsum and segcum are parameters of the calculate_optimization function since these could be derive from seg within the function?

See line 365 of the smart_charging module

segsum = sum(seg)
segcum = np.cumsum(seg)
linprog_result = calculate_optimization(
    charging_consumption,
    rates,
    elimit,
    seg,
    segsum,
    segcum,
    total_trips,
    kwh,
)

@rouille
Copy link
Collaborator

rouille commented Jul 26, 2022

Other comment out of the scope of this PR. In the smart_charging function. the kwhmi parameter is said to vary with model_year and veh_type that are also parameters of the function. can we have a dictionary that would map (model_year, veh_type) to kwhmi to simplify the calling of the function and also make it less obscure to the user. In your test, kwhmi=0.242, who would come up with that value.

@dmuldrew
Copy link
Collaborator Author

dmuldrew commented Aug 8, 2022

@rouille Yeah, I agree a dictionary is a good idea…we’d probably need to request that from them so we can have more comprehensive tests of the various code pathways.

@dmuldrew
Copy link
Collaborator Author

dmuldrew commented Aug 10, 2022 •
edited
Loading

Latest changes reduce the optimization from around ~20% to ~8% of total runtime:

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
   370     14166    3624525.0    255.9      1.8                      linprog_inputs = calculate_optimization(
   371      7083      14128.0      2.0      0.0                          charging_consumption,
   372      7083      14369.0      2.0      0.0                          rates,
   373      7083      14190.0      2.0      0.0                          elimit,
   374      7083      14488.0      2.0      0.0                          seg,
   375      7083      14036.0      2.0      0.0                          segsum,
   376      7083      14784.0      2.1      0.0                          segcum,
   377      7083      13873.0      2.0      0.0                          total_trips,
   378      7083      14797.0      2.1      0.0                          kwh,
   379                                                               )
   380                                           
   381      7083   12041961.0   1700.1      6.0                      linprog_result = linprog(**linprog_inputs)

so now this single line accounts for almost half the runtime (compared to 18% before):

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
   493                                                                   # copy individual back to newdata if it can be an EV
   494      6804   94103836.0  13830.7     47.2                          newdata.iloc[i : i + total_trips] = individual

@dmuldrew dmuldrew marked this pull request as ready for review August 10, 2022 22:28
@dmuldrew dmuldrew changed the title refactor: refactor calculate constraints refactor: calculate constraints all at once Aug 22, 2022
@dmuldrew dmuldrew requested a review from rouille August 22, 2022 19:42
rouille
rouille reviewed Aug 23, 2022
View reviewed changes
prereise/gather/demanddata/transportation_electrification/data_helper.py
"demanddata",
"transportation_electrification",
"data",
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the change? Just being curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just consistent with the method I used on ECTF, I had some issues with file paths on these files

rouille
rouille approved these changes Aug 23, 2022
View reviewed changes
Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks

@dmuldrew dmuldrew force-pushed the dmuldrew/refactor_calculate_constraints branch from bcd1503 to 90d24a8 Compare August 23, 2022 21:57
@dmuldrew dmuldrew merged commit 8bdd583 into transportation_electrification Aug 23, 2022
@dmuldrew dmuldrew deleted the dmuldrew/refactor_calculate_constraints branch August 23, 2022 22:03
rouille pushed a commit that referenced this pull request Jan 12, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
fc18256
rouille pushed a commit that referenced this pull request Jan 19, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
2444d1c
rouille pushed a commit that referenced this pull request Jan 20, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
6bd4c8e
rouille pushed a commit that referenced this pull request Feb 1, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
8f6dc83
rouille pushed a commit that referenced this pull request Feb 1, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
86a6a89
jenhagg pushed a commit that referenced this pull request Feb 23, 2023
@dmuldrew @jenhagg
refactor: calculate constraints all at once (#300)
4a78de6
jenhagg pushed a commit that referenced this pull request Feb 24, 2023
@dmuldrew @jenhagg
refactor: calculate constraints all at once (#300)
e139eeb
rouille pushed a commit that referenced this pull request Feb 24, 2023
@dmuldrew @rouille
refactor: calculate constraints all at once (#300)
70e0725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouille rouille rouille approved these changes

@merrielle merrielle Awaiting requested review from merrielle

@BainanXia BainanXia Awaiting requested review from BainanXia

@jenhagg jenhagg Awaiting requested review from jenhagg

Assignees

@dmuldrew dmuldrew

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmuldrew @rouille